-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add MongoOption builder logic #2623
Conversation
b661080
to
0875e3a
Compare
e61706c
to
956b500
Compare
a545f57
to
dcc1e8f
Compare
dcc1e8f
to
f311817
Compare
22bdd05
to
5816fa2
Compare
5cdd41b
to
b1fca16
Compare
82d2d81
to
3b0430e
Compare
3b0430e
to
956c6a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, lot's to review! Did a first pass and had a few questions.
93df27c
to
73988a7
Compare
static merge( | ||
creds: MongoCredentials, | ||
options: Partial<MongoCredentialsOptions> | ||
): MongoCredentials { | ||
return new MongoCredentials({ | ||
username: options.username ?? creds.username, | ||
password: options.password ?? creds.password, | ||
mechanism: options.mechanism ?? creds.mechanism, | ||
mechanismProperties: options.mechanismProperties ?? creds.mechanismProperties, | ||
source: options.source ?? creds.source ?? options.db | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbroadst
Sorry about my git fu, I can't reply to your comment about the credentials issue. I've gone ahead with the merge solution, lmk what you think. For what its worth I think we were safe with the spread solution because typescript would not permit us to have a property missing, it just so happens that MongoCredentials and MongoCredentialOptions share all their properties and mandate that they are defined, but ofc better to be safe and box up more complex functionality, so .merge
seems like a good solution .
73988a7
to
1dd1b8d
Compare
Adds MongoOptions representing the internal view of the specified options. NODE-2698
Parse options from uri and object creating frozen MongoOptions interface object. NODE-2699
1dd1b8d
to
fe8dffd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏 👏
MongoClientOptions.useNewUrlParser has been removed: mongodb/node-mongodb-native#2623
Parsing options from URI and options object we get a final MongoOptions shaped object, one downside here is that the building process isn't typechecked, doesn't seem like there's an easy way to do that. But the final product is an options property on MongoClient that carries all the top level options specified by the user.
A part of this work revealed a few issues:
minPoolSize
andminSize
might supersede one another and there's no clear way to give priority but also to communicate that priority. I removed the aliasesminSize
andpoolSize
.ha
,haInterval
,domainsEnabled
,useUnifiedTopology
,useNewUrlParser
,appname
(notappName
), andvalidateOptions
NODE-2699, NODE-2871